RPC and WS endpoints share the same port #187

dimalinux · 2022-09-08T18:00:19Z

This PR combines our HTTP RPC and Websocket services into a single HTTP server, eliminating the --ws-port flag to swapd. This lowers end-user complexity, especially when running multiple swapd servers on one host. swapcli subscribe interactions are fixed with this PR and the --daemon-addr http://127.0.0.1:PORT flag to swapcli was removed. Instead of passing separate http and websocket URLs to swapcli, swapcli commands now use --swapd-port PORT_NUM allowing swapcli to interact with both services, while only requiring one flag from the end user.

dimalinux · 2022-09-08T18:04:18Z

cmd/daemon/main.go

-			d.cancel()
-			os.Exit(1)
-		}
-	}()


Start() now blocks and it internally uses the context that was passed to rpcCfg.

dimalinux · 2022-09-08T18:09:24Z

common/coins_test.go

 	val = NewEtherAmount(1234567890)
-	require.Equal(t, fmt.Sprint(val.ToDecimals(6)), "1234.56789")


The expected/actual values here were reversed and it seemed like as good a place as any to use the FmtFloat function added by this PR. I've never used fmt.Sprint like above, but I imagine it behaves like "%v" in a formatted string, which in some cases prints undesirable exponent notation.

dimalinux · 2022-09-08T18:17:26Z

rpc/mocks_test.go

+
+//
+// This file only contains mock definitions used by other test files
+//


These mocks were previously located in ws_test.go. I didn't realize that they were also being shared with net_test.go, so I thought pulling them out into a separate file might make make it clear that they are shared. I put panics in most of them, to make it more obvious to the reader which functions are actually getting invoked. If we decide to instrument the mocks with testify's mocking at a later time (https://github.com/stretchr/testify#mock-package), we wouldn't need to instrument all functions.

I'm also curious if it might be worth an attempt to update these tests so that they don't use mocking at all?

it's a lot more complex to update the tests to not use mocking, as we basically need to start up everything in the daemon. it might be nice to add them as integration tests, instead of unit tests.

dimalinux · 2022-09-08T18:20:05Z

rpc/mocks_test.go

+		StatusCh: make(chan types.Status, 1),
+		InfoFile: "/dev/null",
+	}
+	offerExtra.StatusCh <- types.CompletedSuccess


I uncommented the TestSubscribeMakeOffer test. I'm open to better ideas on how to transition the state for it.

dimalinux · 2022-09-08T18:28:29Z

rpc/server.go

@@ -43,67 +42,71 @@ type Config struct {

 // NewServer ...
 func NewServer(cfg *Config) (*Server, error) {


I pulled all the things that can fail in the lead-up to launching the server out of the Start() method and into this NewServer method. One of the things is allocating the listener, as that allows us to pass 0 for the port and have the OS assign us a free ephemeral port. This is useful when testing, when the actual port used is unimportant, but it must be a free port. We want to be able to query the allocated port back from the Server object before calling the blocking method Start().

dimalinux · 2022-09-08T18:33:28Z

rpc/server.go

+	originsOk := handlers.AllowedOrigins([]string{"*"})
+	server := &http.Server{
+		Addr:              ln.Addr().String(),
+		ReadHeaderTimeout: time.Second,


Interestingly, the linter flagged this as vulnerable to the Slowloris attack (https://www.cloudflare.com/learning/ddos/ddos-attack-tools/slowloris/) without the ReadHeaderTimeout value, but as best I can tell the original ListenAndServe call we were using would also be vulnerable, but doesn't get flagged by the linter. I guess because there is not API to set it.

dimalinux · 2022-09-09T05:22:39Z

net/discovery.go

+		return nil, ctx.Err()
+	default:
+		// not cancelled, continue on
+	}


This was mildly painful. We were introducing a context cancel in the TestDaemon_DevXMR[MT]aker tests in cmd/daemon to stop the http server and it was triggering a panic in this libp2p code. Apparently, this libp2p code has really slow startup times, as I even tried sleeping for a 2 whole seconds before issuing the context cancel and I was still getting panics.

good to know, also yeah that's painful

dimalinux · 2022-09-09T05:29:50Z

ui/src/components/TakeDealDialog.svelte

@@ -14,7 +14,7 @@
  import HelperText from '@smui/textfield/helper-text'
  import { currentAccount, sign } from '../stores/metamask'

-  const WS_ADDRESS = 'ws://127.0.0.1:8081'
+  const WS_ADDRESS = 'ws://127.0.0.1:5001/ws'


I suspect this is the only change needed, but since the UI doesn't work for me, I can't test it.

dimalinux · 2022-09-09T18:58:14Z

cmd/client/main.go

 				},
 			},
 		},
-		Flags: []cli.Flag{daemonAddrFlag},


I wanted users to be able to specify --swapd-port before or after the subcommand. The spf13/cobra library makes this easy to do, but I guess it is not supported by urfave: urfave/cli#1391

When we define the same flag globally and after the command, the behavior is bad. I wasn't getting an error for the global flag (since it was listed as a global flag), but when reading it, we would get the defaulted value from the flag to the subcommand instead of the globally set value.
Example:

./swapcli --swapd-port 5432 address

The actual port used would be 5001, because the defaulted value for --swapd-port after the address subcommand was taking precedence over the 5432 value.

cmd/client/main.go

noot

looks good! could you also update the docs to reflect the changes (local.md, rpc.md, and stagenet.md)?

dimalinux · 2022-09-09T20:53:34Z

rpcclient/wsclient/wsclient.go

-	TakeOfferAndSubscribe(multiaddr, offerID string,
-		providesAmount float64) (id uint64, ch <-chan types.Status, err error)
+	SubscribeSwapStatus(id types.Hash) (<-chan types.Status, error)
+	TakeOfferAndSubscribe(multiaddr, offerID string, providesAmount float64) (ch <-chan types.Status, err error)


I didn't change these functions. The interface signatures were just out of date with the existing implementation.

noot

looks good to me!

Combine our HTTP RPC and Websocket services into a single HTTP server, eliminating the --ws-port flag to swapd and using --swapd-port flag for swapcli.

RPC and WS endpoints share the same port

4b7c101

dimalinux commented Sep 8, 2022

View reviewed changes

dimalinux marked this pull request as draft September 8, 2022 19:14

dimalinux added 3 commits September 8, 2022 23:38

updated daemon unit tests to work with the new code behavior

7fed2b7

typo fix

513e359

updated websocket URL in the web UI

b71a89e

dimalinux commented Sep 9, 2022

View reviewed changes

dimalinux added 2 commits September 9, 2022 12:03

added needed wallet.Refresh() calls to TestClient_Transfer

1c72189

CLI tweaks

8fc945b

dimalinux commented Sep 9, 2022

View reviewed changes

dimalinux marked this pull request as ready for review September 9, 2022 18:59

dimalinux requested a review from noot September 9, 2022 18:59

noot reviewed Sep 9, 2022

View reviewed changes

cmd/client/main.go Outdated Show resolved Hide resolved

noot reviewed Sep 9, 2022

View reviewed changes

dimalinux commented Sep 9, 2022

View reviewed changes

dimalinux added 3 commits September 9, 2022 20:49

fixed typo in printOfferSummary name

c85fe15

updated docs with new --swapd-port flag and ws://IP/ws websocket URLs

50b9fb4

updated websocket URLs in testerconfig.json

0cb5c81

noot approved these changes Sep 10, 2022

View reviewed changes

dimalinux merged commit 7f9258a into master Sep 10, 2022

dimalinux deleted the dimalinux/shared-rpc-ws-port branch September 10, 2022 19:54

noot pushed a commit that referenced this pull request Oct 2, 2022

RPC and WS endpoints share the same port (#187)

5698e25

Combine our HTTP RPC and Websocket services into a single HTTP server, eliminating the --ws-port flag to swapd and using --swapd-port flag for swapcli.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC and WS endpoints share the same port #187

RPC and WS endpoints share the same port #187

dimalinux commented Sep 8, 2022

dimalinux Sep 8, 2022

dimalinux Sep 8, 2022

dimalinux Sep 8, 2022 •

edited

Loading

noot Sep 9, 2022

dimalinux Sep 8, 2022

dimalinux Sep 8, 2022

dimalinux Sep 8, 2022

dimalinux Sep 9, 2022

noot Sep 9, 2022

dimalinux Sep 9, 2022

dimalinux Sep 9, 2022

noot left a comment

dimalinux Sep 9, 2022

noot left a comment

		val = NewEtherAmount(1234567890)
		require.Equal(t, fmt.Sprint(val.ToDecimals(6)), "1234.56789")

		@@ -43,67 +42,71 @@ type Config struct {

		// NewServer ...
		func NewServer(cfg Config) (Server, error) {

RPC and WS endpoints share the same port #187

RPC and WS endpoints share the same port #187

Conversation

dimalinux commented Sep 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimalinux Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noot left a comment

Choose a reason for hiding this comment

dimalinux Sep 8, 2022 •

edited

Loading